Skip to content

Make "Assemble stage1 compiler" orders of magnitude faster (take 2) #97268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 22, 2022

This used to take upwards of 5 seconds for me locally. I found that the culprit was copying the downloaded LLVM shared object:

[22:28:03] Install "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/ci-llvm/lib/libLLVM-14-rust-1.62.0-nightly.so" to "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libLLVM-14-rust-1.62.0-nightly.so"
[22:28:09]   c Sysroot { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu) } }

It turned out that install() used full copies unconditionally. Change it to try using a hard-link before falling back to copying.

I also took the liberty of fixing x dist llvm-tools to work even if you don't call x build previously.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2022
@jyn514 jyn514 force-pushed the faster-assemble branch from 9d4a495 to c5d6a75 Compare May 22, 2022 21:54
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 force-pushed the faster-assemble branch from c5d6a75 to 460ad72 Compare May 22, 2022 22:11
@apiraino apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label May 23, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented May 23, 2022

📌 Commit 460ad720eacb3c5eb6522ddf2f31826969809e9b has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2022
@Mark-Simulacrum
Copy link
Member

@bors r-

Let's hold off until we confirm the permissions strategy here.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 23, 2022
@jyn514 jyn514 force-pushed the faster-assemble branch from 460ad72 to cc9cea4 Compare May 26, 2022 15:50
@jyn514
Copy link
Member Author

jyn514 commented May 26, 2022

Ok, I updated the PR to:

  • Panic if we generate a symbolic link in a tarball
  • Change install to use copy internally, like in my previous PR
  • Change copy to dereference symbolic links, which avoids the previous regression.

There is no more special casing file.is_symlink or llvm_from_ci anywhere in dist. In terms of permissions, install will now change the permissions of both the source and destination when using hard links; we control the source files, so I don't think it will be an issue.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2022
@jyn514
Copy link
Member Author

jyn514 commented May 26, 2022

Panic if we generate a symbolic link in a tarball

I tested this by cherry-picking b2f0e38ffe210a0099faad9b1594feb6fcf0b2da onto #96803 and making sure that x dist llvm-tools panicked.

@jyn514 jyn514 force-pushed the faster-assemble branch from cc9cea4 to fb46976 Compare May 26, 2022 16:01
@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Collaborator

bors commented May 28, 2022

⌛ Trying commit fb469766fcc0d11aa86736a677fca4a18bf43be9 with merge 4e819be6a73e5fd42d55a1f5d61c69264ae74cbc...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 28, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2022
@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

This error is strange. src/tools/cargo/tests/testsuite/cargo_add/add-basic.in doesn't exist in tree, but the error doesn't say "file not found". Maybe it's generated somehow? I don't know in which step this is going wrong ...

@epage do you know how add-basic.in is generated?

@jyn514
Copy link
Member Author

jyn514 commented May 29, 2022

Strangely, I can't seem to find a component that rustup-toolchain-install-master can't install - I tried -c rustc-src and -c rustc-nightly-src, which both say file not found, and -c rust-src works fine.

Looks like neither rustup nor rustup-toolchain-install-master are able to install it - they assume the target has x86_64-unknown-linux-gnu, but it doesn't for this source tarball.

$ curl -I https://ci-artifacts.rust-lang.org/rustc-builds/bef2b7cd1c7bcb3393f10d5752fcf9ee3026bce8/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz
HTTP/2 200 
$ curl -I https://ci-artifacts.rust-lang.org/rustc-builds/bef2b7cd1c7bcb3393f10d5752fcf9ee3026bce8/rustc-nightly-src.tar.xz
HTTP/2 200 
$ rustup component add rustc-src --target ''
error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not contain component 'rustc-src' for target ''; did you mean 'rust-src'?

I'm going to go ahead and special-case it.

@jyn514 jyn514 force-pushed the faster-assemble branch from ee3b7e8 to 3213672 Compare May 29, 2022 20:34
@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

@bors try

@bors
Copy link
Collaborator

bors commented Jun 8, 2022

⌛ Trying commit 3213672cf8e60bb8440700bfc8e553a81a08de20 with merge bc0fb8974a74c2c48b91bde70bd794ee2998a310...

@bors
Copy link
Collaborator

bors commented Jun 8, 2022

☀️ Try build successful - checks-actions
Build commit: bc0fb8974a74c2c48b91bde70bd794ee2998a310 (bc0fb8974a74c2c48b91bde70bd794ee2998a310)

@jyn514
Copy link
Member Author

jyn514 commented Jun 8, 2022

Ok cool! This should be ready for review then.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2022
@Mark-Simulacrum
Copy link
Member

r=me with comments resolved (let me know if you feel there's a need for another review, but I expect resolution to be pretty non-noteworthy)

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2022
@jyn514 jyn514 force-pushed the faster-assemble branch 2 times, most recently from 52e52e7 to 3905e37 Compare June 19, 2022 20:51
jyn514 and others added 2 commits June 19, 2022 15:54
This avoids regressions in rustup-toolchain-install-master
This used to take upwards of 5 seconds for me locally. I found that the
culprit was copying the downloaded LLVM shared object:
```
[22:28:03] Install "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/ci-llvm/lib/libLLVM-14-rust-1.62.0-nightly.so" to "/home/jnelson/rust-lang/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libLLVM-14-rust-1.62.0-nightly.so"
[22:28:09]   c Sysroot { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu) } }
```

It turned out that `install()` used full copies unconditionally. Change
it to use `copy()` internally, which uses hard links instead when
available.

Note that this has a change in behavior: Installing a file will also
change permissions on the source, not just the destination, if hard
links are used.

To avoid changing the behavior on symlinks for existing code, I
introduce a new function `copy_internal` which only dereferences
symlinks when told to do so.
@jyn514 jyn514 force-pushed the faster-assemble branch from 3905e37 to 057eab7 Compare June 19, 2022 20:54
@jyn514
Copy link
Member Author

jyn514 commented Jun 19, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Jun 19, 2022

📌 Commit 057eab7 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2022
@bors
Copy link
Collaborator

bors commented Jun 19, 2022

⌛ Testing commit 057eab7 with merge 611e7b9...

@bors
Copy link
Collaborator

bors commented Jun 20, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 611e7b9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 20, 2022
@bors bors merged commit 611e7b9 into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
@jyn514 jyn514 deleted the faster-assemble branch June 20, 2022 00:42
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (611e7b9): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-3.7% -3.7% 1
Improvements 🎉
(secondary)
-3.7% -4.7% 3
All 😿🎉 (primary) -3.7% -3.7% 1

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.1% 3.1% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants